Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for phone and email #174

Closed
wants to merge 1 commit into from
Closed

Conversation

adrianba
Copy link
Contributor

This is a proposed solution for issue #1 and a counter proposal to PR
#65.

This change adds support for two new PaymentOptions for payer's email
and payer's phone number. These values are provided on the
PaymentResponse object. Since these values should not change the total
price, no PaymentRequestUpdate event is fired for them.

The change also adds a phone number to the ShippingAddress data
structure with the implication that this gets populated when
requestShipping is true.

Here are some variations to this change that should be considered:

This is a proposed solution for issue w3c#1 and a counter proposal to PR
w3c#65.

This change adds support for two new `PaymentOptions` for payer's email
and payer's phone number. These values are provided on the
`PaymentResponse` object. Since these values should not change the total
price, no `PaymentRequestUpdate` event is fired for them.

The change also adds a phone number to the `ShippingAddress` data
structure with the implication that this gets populated when
`requestShipping` is `true`.

Here are some variations to this change that should be considered:
* Do not include `requestPayerPhone` (issue w3c#1 actually only mentions
shipping phone number)
* Do include a `requestShippingPhone` value and only populate the
`ShippingAddress.phone` value if both `requestShipping` and
`requestShippingPhone` are true.
@adrianba
Copy link
Contributor Author

One advantage to using boolean types here is that we could change them in the future to be an enum that allowed for required/optional/notRequired values. I'm proposing that we start with boolean to keep it simple and get implementation experience.

@ianbjacobs
Copy link
Collaborator

@adrianba,

One advantage to using boolean types here is that we could change them in the future to be an enum that allowed for required/optional/notRequired values.

Is the implication that changing from boolean to some other values is a low cost change from an implementation standpoint?

Ian

@adrianba
Copy link
Contributor Author

adrianba commented May 2, 2016

@ianbjacobs

Is the implication that changing from boolean to some other values is a low cost change from an implementation standpoint?

I'm not implying anything about cost (there are definitely UI challenges for example) but I am implying that it is possible to do this later in a more-or-less backwards compatible way.

@zkoch
Copy link
Contributor

zkoch commented May 4, 2016

This PR LGTM. I'm also okay as a starting point with leaving out "requestPayerPhone" but I do think there are a variety of situations where it would be useful (e.g. hotel bookings, where a shipping address is not requested, but a phone # is).

@rvm4
Copy link
Collaborator

rvm4 commented May 10, 2016

This sounds fine to me, but I don't understand why you have added phone number to the address, but not email. Neither seems more relevant to the address and I don't think either one has a stronger use case than the one. Can we add both to the address, or create some other Contact structure?

@mattsaxon
Copy link
Contributor

Happy to merge. Will update comment with more detailed outcome later.

@adrianba
Copy link
Contributor Author

Merged as 31346cf after last telcon and further discussion with @mattsaxon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants